feat(cli): add ACP live session lifecycle - #4862

Thanks for implementing the live ACP lifecycle. I reviewed 8e56e03c with three focused reviewer passes and independent reconciliation. Reusing the Runtime Host channel/projector and keeping execution in the Host is the right direction. Two recoverable P2 cancellation/teardown cases remain inline.

Please keep the fix in the existing prompt/attachment lifecycle: observation ending is not proof that a dispatched Turn was never admitted, and ACP cancellation completion is distinct from successful delivery of Host Stop. No extra cancellation authority or global timeout framework is needed. Repeated exact Stop is already idempotent at the Host; the documented append-only stream-revision limitation is not an additional finding.

Validation: 59 focused checks passed across the targeted runs, plus two independent current-SHA probes reproducing the inline cases. No fresh full CLI build or Zed acceptance run. AI-assisted review; primary-agent reconciliation.

Thanks for the fixes. I rechecked 1ef5b621 and confirmed that both previous P2s are resolved. I found one adjacent cancellation gap and have left it inline. It also reproduces on 8e56e03c; I missed it in the earlier review, so this is not a regression introduced by the latest fixes.

AI-assisted review: 66 focused checks passed, and seven regression checks failed against the previous registry implementation. The additional registry probe below fails on both heads. I did not repeat the Zed smoke test.

Automated review (Command Code) — not an approval

The lifecycle design is sound and the ACP surface correctly reuses the existing channel/projector rather than forking a parallel path. One defect contradicts the PR's own recovery claim, and one leaves a reachable class of turns unable to complete. Line numbers are the head revision.

P1 (Must-Fix) — the connection wrapper drops reconnecting, so every recoverable Host error fails the attachment instead of resyncing.

runMakaAcpStdioServer validates the connection and then hands downstream a new object literal:

// packages/cli/src/acp/stdio-server.ts:56-65
if (!isRuntimeHostReconnectingConnection(connection)) { ... throw ... }
return {
  request: connection.request.bind(connection) as RuntimeHostConnection['request'],
  openSessionSubscription: connection.openSessionSubscription.bind(connection),
  openSessionSubscriptionOnce: connection.openSessionSubscriptionOnce.bind(connection),
  close: () => context.close(),
};

That literal omits reconnecting. The type it satisfies (AcpSessionRegistryConnection, packages/cli/src/acp/session-registry.ts:92-96) Picks only 'request' | 'openSessionSubscription' | 'openSessionSubscriptionOnce' | 'close', so the omission type-checks. The registry passes that same object to the channel (openRuntimeHostSessionAttachment, session-registry.ts:897-898), and the channel's recovery gate is exactly that marker:

// packages/cli/src/runtime-host-session-channel.ts:590-591
#canRecover(error: unknown): boolean {
  if (!isRuntimeHostReconnectingConnection(this.#connection)) return false;

// packages/runtime-host/src/client/reconnecting-connection.ts:112-116
export function isRuntimeHostReconnectingConnection(connection: unknown): connection is ... {
  return (connection as Partial<...>).reconnecting === true;
}

this.#connection is that literal (runtime-host-session-channel.ts:98, 137), so #canRecover is always false on the ACP path. Every recoverable condition (connection_closed, sequence_gap, projection_revision_invalid, transcript_release_failed, slow_consumer) is routed to #fail(error) instead of #recover(...), which fires onFailed → #retireFailedAttachment → failTurn on the active prompt. A Host restart or a slow_consumer eviction therefore fails the live session/prompt with an internal error instead of resyncing — the opposite of the stated "recovery uses the existing reconnecting path". The wrapper even binds openSessionSubscriptionOnce correctly, so recovery would work; only the gate is defeated.

Why CI cannot see it: the only other channel owner (the TUI driver) passes the real impl instance, where reconnecting = true is an own property. In this package, reconnecting appears outside tests only on the impl class — the ACP tests inject it on the mocked context.connection (packages/cli/src/__tests__/acp-stdio-server.test.ts:311), i.e. before production re-wraps it, and the registry's own fake omits it entirely. No test drives session/prompt through the real stdio wrapper.

Smallest sound fix: widen the Pick to include 'reconnecting' and set reconnecting: true on the returned literal (two lines). A test that runs a prompt through the real wrapper would also close the blind spot.

P2 (Should-Fix) — interaction requests are discarded, and no ACP path can answer one, so an affected turn cannot complete.

The attachment wires the interaction callbacks to no-ops:

// packages/cli/src/acp/session-registry.ts:906-907
onInteractionPending: () => undefined,
onInteractionResolved: () => undefined,

and the event mapper handles only text_delta | text_complete | thinking_delta | thinking_complete | complete | error | abort, with default: break (packages/cli/src/acp/session-event-mapper.ts:85). So user_question_request, form_request and sandbox_boundary_request are dropped, and there is no session/request_permission or interaction-answer call anywhere under packages/cli/src/acp/. The turn has a permission_mode config option, so a client can select ask; a turn that then raises an interaction has no path to a settlement. Deferring interactions to a follow-up is a legitimate scope decision — leaving the affected prompt with no client-visible signal is not. Suggest turning onInteractionPending into an explicit turn failure with a clear unsupported_interaction code (the PR already uses that fail-with-diagnostic shape when Stop delivery fails), so the client sees an error instead of nothing.

Note this is permission-adjacent surface, but the diff does not bypass any permission or sandbox decision — the defect is fail-to-respond, not fail-open.

Review-relevant risks. The P2 surface touches permissions, so an independent human should look at it under CONTRIBUTING.md. No credential, release, licensing or governance effect was identified.

Required conclusion.

1. Optimal for the actual problem? Largely yes for the prompt/stream/cancel/close core; not optimal given P1 — one missing marker disables the capability the design documents.
2. Production code that can be deleted? none identified.
3. Low-quality tests to delete or replace? none identified. The gap is coverage, not quality: the production connect() wrapper, a recoverable subscription error on an ACP attachment, and any interaction becoming pending are all untested.
4. Deeper refactor required? No. Two targeted changes: propagate the marker, and stop swallowing interactions.
5. Ready to merge? Not as-is. P1 is a one-line fix that contradicts a central claim of the PR; P2 needs at least an explicit failure path.
6. Residual risks / verification gaps: I did not run the suite (the PR's pass counts are unverified). A cancelled turn can keep emitting agent_message_chunk after the prompt already returned cancelled — off-spec once the response is sent, unverified against a real client.

Approval boundary. This is automated review; it is not an approval. Per CONTRIBUTING.md, the merge decision requires an independent human review. No approve was submitted.

Follow-up (Command Code) — verified resolved

I re-checked the head after your push.

The P1 is fixed. The connection wrapper returned from the stdio server now carries reconnecting: true alongside the bound methods, so isRuntimeHostReconnectingConnection returns true and the channel's recovery gate can engage — the ACP path now gets the resync behaviour the design describes instead of failing the attachment.

The P2 is addressed too: the interaction callbacks are no longer no-ops, so a pending interaction is no longer silently discarded with the prompt left hanging.

Thanks for turning these around. (Automated review; not an approval.)

Reviewed ccaba9876. Architecture: the seam is right. Durable ownership (#ownedSessionIds) is separate from the single-flight attachment (#attachments), the first prompt attaches and starts eventsForTurn before turn.start, capacity stays with Host admission, and the registry consumes RuntimeHostSessionChannel through the same open/eventsForTurn/failTurn/snapshot surface the TUI driver uses. Nothing re-derives Turn state: cancellation and Stop always take identity from the subscription root, the turn.start result or turn.query. All five prior findings are fixed and their regressions go red when the fixes are reverted.

Not approvable yet: two P2s remain, both local to session-registry.ts, both on ② recovery paths. Nothing at P0/P1.

P2 — Unknown admission never settles when recovery is permanently dead; prompt, cancel, close and dispose wedge

Reachability ②: turn.start dispatched, response lost, then reconnect fails permanently (RuntimeHostPermanentReconnectError) or the Host never returns before client EOF.

session-registry.ts:352-362 settles unknown admission only on not_found; onFailed → #retireFailedAttachment (:536-542, :570-587) fails the queue and wakes the prompt but leaves admissionSettled=false, so #stopPromptWhenObservable (:476-477) waits again with no fact source left. #cancelPrompt awaits that task, and #dispose (:811-813) awaits cancellations before #closeOwnedConnection, so the query can only be rejected by the close that dispose never reaches.

Repro (registry fixture): prompt → reject start with RuntimeHostRequestInterruptedError('turn.start','command','dispatched','connection_lost') → reject turn.query with a non-not_found error → attachment.failAttachment(...). Prompt, cancel and dispose() stay pending; Stop count 0.

Fix inside the existing attempt: in #retireFailedAttachment set admissionSettled = true for matching prompts before #wake; likewise settle when turn.query rejects with anything the reconnecting connection will not retry, keeping only connection_lost/timeout as "still unknown". Add the permanent case to the retains cancellation … unknown admission matrix. Reordering #dispose also unwedges shutdown, but settling the attempt is the root fix.

P2 — Configuration refresh failure during a live prompt stops the Host Turn

Reachability ②, triggered from ①: the Host bumps the session header during a normal first Turn (execution-composition.ts:1014-1016 nameSessionIfUnnamed), so metadataRevision changes while streaming and onSnapshotChanged (session-registry.ts:509-525) reads both catalogs. Any failure there is routed to #retireFailedAttachment (:521-525), which failTurns the active prompt; #prompt's catch then sends turn.stop for the running Turn (:379-381) and the client gets an unmapped Internal error.

Repro (registry fixture): prompt with root running → make session.catalog.query throw once → attachment.setMetadataRevision(2). Prompt rejects with the raw error, turn.stop called once, attachment closed.

This contradicts #create's own stance (:654-662: projection failure must not fail a committed Host result) and the checklist, which makes config_option_update a presentation notification. Fix: make the refresh best-effort — log and keep the attachment and prompt; the next metadataRevision change retries. That also deletes the earlyFailure branch on this path. Keep retirement for genuine channel failures (onFailed).

Complexity

openSessionSubscriptionOnce (+22 lines across four files) differs from openSessionSubscription only in fail-fast vs wait-for-reconnect on the first attach, since the lifecycle already rejects root/protocol/composition changes permanently (reconnecting-connection.ts:78-102). It is what the #3132 checklist asks for; noted so the cost is explicit. session-event-mapper.ts:80 re-derives terminal classification the channel already owns (inline). The tail/retired/delivery serialization is the minimum for the ordering the checklist demands.

Tests: node --test --test-concurrency=4 "dist/__tests__/acp-*.test.js" 102 pass (incl. 10 real Runtime Host child-process tests); reconnecting-connection.test.js 25 pass. Not repeated: full CLI suite, Zed smoke.

Reviewed ecfe143f8. Both P2s are fixed inside the existing prompt attempt and every new regression goes red with its production change reverted; my inline on session-event-mapper.ts:80 was wrong and is withdrawn (the channel finishes a Turn queue only on Host terminal facts, and the kernel treats error as non-terminal). Not approving yet, for a reason I own part of: the registry now carries about 1.1k production and 2.7k test lines for prompt/stream/cancel/close, and a large share of that is a state machine the channel already provides — including the unknown-admission latch my earlier rounds asked to harden rather than remove.

P2 — The unknown-admission latch duplicates what the channel snapshot already gives

Reachability ② (lost turn.start response). ActiveAcpPrompt.{admissionSettled, startRequestSettled, startedTurn, waiters}, the turn.query fallback (session-registry.ts:349), the while (!finished) loop in #stopPromptWhenObservable (:455-481), #waitForPromptChange (:637), the latching in #retireFailedAttachment and the unknownAdmissions block in #dispose exist so a prompt whose start response was lost stays open until a later cancel can Stop the exact Turn. The TUI driver on the same channel does not have this: it failTurns and throws on a start failure (runtime-host-session-driver.ts:385-388) and takes Stop identity from channel.snapshot.rootTurn at stop time (:1084-1099). turn.start is never replayed by the reconnecting connection, so the latch only buys waiting for recovery to reveal the root, and the registry already stops an observed non-terminal root on an idle attachment in #cancelSession (:414-437); close cancels first. Replace the latch with: fail the prompt with request_interrupted; if cancelled and turn.start resolved started, Stop result.turn; otherwise Stop snapshot.rootTurn when it matches. The checklist's "deterministic termination on subscription failure" and "cancel via snapshot identity" are both met by that. About −120 production, −400 test (acp-session-registry.test.ts:584-981). I asked for the opposite in the :451 round; the driver comparison is what changes my view.

P2 — Registry tests run against a hand-written channel, not the channel

AcpSessionAttachment / openSessionAttachment (session-registry.ts:99, :123, :179) have one production implementation and exist so acp-session-registry.test.ts can inject FakeAcpSessionAttachment (:2933, ~270 lines); the test's fakeConnection throws on both subscription opens, so ~1000 lifecycle test lines never touch RuntimeHostSessionChannel. Those tests re-specify when rootTurn appears and when queues finish instead of asserting the channel's behaviour, which is how the unknown-admission and configuration-ordering mechanisms grew without a production counterexample. The stdio-server and driver tests already drive the real channel through FakeSubscription; the registry tests can use the same harness. Delete the two interfaces, the option and the fakes; keep the real-connection tests.

P3 — Smaller cuts

• Configuration promise queue (tail/retired/delivery/#pendingConfigSets, :130-136, :165-228, :491-500): metadataRevision and the CAS revision are the same header revision, so a last-published-revision compare orders sets against refreshes with no queue; retired is never awaited. About −65 / −290.
• openSessionSubscriptionOnce (reconnecting-connection.ts:49, :219; channel :72, :169; stdio-server.ts:64; registry :929): it pins the epoch only at call time; the following turn.start and every later channel #recover use whatever epoch is current, exactly as the retrying open does. The checklist line it implements ("attachment never silently lands on a replacement Host epoch") is not a guarantee this code provides, and the TUI does not have it. Open the channel like the driver and drop the extra path (~70 lines).
• resource_link admission (prompt-content.ts, ~120 of 149 lines + 174 test): the checklist asks for "supported ACP prompt content"; is there evidence a real client (the Zed smoke) sends resource_link? If not, text-only meets PR4 and attachments return with a client that uses them.
• packages/cli/src/acp/README.md restates error codes and behaviour that live in the registry; every later checklist PR will change it. Drop.

A PR4 with those cuts is roughly 450 production and 700 test lines for the same checklist.

Tests on this head: node --test --test-concurrency=4 "dist/__tests__/acp-*.test.js" 127 pass. Not repeated: full CLI suite, Zed smoke.

Thanks for reworking the tests through the real Session channel and for the concrete counterexamples to the earlier deletion requests. I re-reviewed 0ba09673387cbde651b9474fdd5a313cc6b5f6a0 with separate architecture/ablation and implementation passes. One P2 cancellation edge remains, noted inline.

I am withdrawing the earlier broad recommendation to remove the unknown-admission latch and configuration queue. The ACP request/response obligations are not equivalent to the TUI driver's: cancellation can need an identity that appears only after recovery, and asynchronous r2 projection must retain its notification/response order when an external r3 update arrives. These are request/delivery coordination concerns, while the channel/projector remains the sole authority for recovery, sequence, and Turn termination.

The other retained pieces also have concrete contracts: the tracker requires one-shot initial attachment at the reconnecting boundary; the SDK baseline requires ResourceLink support; the tracker requires incremental capability documentation. The fake attachment/opener and unread retired state have been removed. I would not repeat those deletion requests or split this connected lifecycle solely because the diff is XXL.

There is one small optional cut: AcpPromptContentDependencies.openFile exists only for the oversized-file test. A sparse temporary file larger than the limit could exercise the real filesystem seam and remove that test-only dependency surface. This is a small maintenance improvement, not a blocker or a reason for another abstraction.

The remaining inline issue is narrower than the prior unknown-admission fix: a failed recovery query is still being treated as permission to release the cancellation owner before the channel can recover the running Turn. Please preserve deterministic settlement on a genuinely failed attachment while fixing this case through the existing channel lifecycle.

Validation: an exact-snapshot build and a focused production stdio/RuntimeHostSessionChannel probe reproduced a dispatched start losing its reply, followed by turn.query returning internal_failure: the prompt returned cancelled with zero Stops, and later subscription recovery still produced zero Stops for the running root. Current GitHub checks are successful/skipped. The complete suite and Zed UI smoke were not repeated in this review.

AI assistance: Codex coordinated two deep reviewers and cross-checked the production path and focused reproduction.

Thanks for addressing the recovery boundary and adding production-path regressions. I re-reviewed 87b1572 against the PR4 scope in #3132, including the changes since 0ba0967. No P0–P2 findings remain; I resolved the two outstanding unknown-admission threads.

A failed recovery query no longer means admission was absent. The existing prompt attempt retains cancellation while subscription/query evidence can provide the exact Turn identity. Query exhaustion has an explicit outcome_unknown failure boundary, rather than silently claiming the Host stopped or waiting forever. Initial attachment hydration can now be cancelled at EOF, with late subscriptions cleaned up; an idle attachment also retains external-Turn identity for cancel/close.

The resource-link path now uploads through the existing Host Artifact protocol before Turn admission and uses the canonical returned attachment. I checked cancellation/staging cleanup and the real SDK → ACP child process → Host path. Host remains the execution and terminal-state authority; the adapter owns request, delivery, and bounded recovery coordination. I found no justified broader rewrite or additional authority to introduce. The previously noted test-only openFile cleanup remains an optional P3 and is not a reason to hold this fix.

Validation: Runtime Host and CLI builds; 86 registry/stdio tests; 10 prompt-content tests; and the real-child-process resource-link case passed (97 distinct tests). The child-process check used Node 24 after correcting the temporary checkout's dependency resolution. Exact-head CI and all non-skipped CLI packaging/platform checks are green; the PR is mergeable. I did not rerun the full workspace suites or Zed UI smoke.

AI assistance: Codex coordinated a Reviewer Sol lifecycle pass and independently cross-checked the changed production paths and focused validation. Approval is submitted under the maintainer's standing instruction to approve reviews with no remaining blocking findings.